-
Notifications
You must be signed in to change notification settings - Fork 876
Improve "Video upload info not found" #1116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughTyped error handling is introduced via AuthedApiError across API/upload code paths. Recording commands now return a RecordingAction result, and frontend routes funnel these through a new handleRecordingResult helper. AuthenticationInvalid is removed. Control flow updates propagate typed auth/upgrade states from backend to UI, adjusting signatures and mappings accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend (Routes)
participant Tauri as TAURI Commands
participant Rec as recording.rs
participant Web as web_api.rs
participant Up as upload.rs
User->>UI: Start recording
UI->>Tauri: startRecording()
Tauri->>Rec: start_recording()
Rec->>Up: create_or_get_video()
Up->>Web: authed_api_request(...)
alt Auth OK
Web-->>Up: 200 OK
Up-->>Rec: S3UploadMeta
Rec-->>Tauri: RecordingAction::Started
else Invalid auth
Web-->>Up: AuthedApiError::InvalidAuthentication
Up-->>Rec: AuthedApiError::InvalidAuthentication
Rec-->>Tauri: RecordingAction::InvalidAuthentication
else Upgrade required
Web-->>Up: AuthedApiError::UpgradeRequired
Up-->>Rec: AuthedApiError::UpgradeRequired
Rec-->>Tauri: RecordingAction::UpgradeRequired
end
Tauri-->>UI: Promise<RecordingAction>
UI->>UI: handleRecordingResult(...)\n- prompt login/switch\n- open Upgrade\n- or proceed
sequenceDiagram
autonumber
participant Up as upload.rs
participant API as api.rs
participant Web as web_api.rs
Up->>API: upload_multipart_initiate()
API->>Web: authed_api_request(...)
alt HTTP error
Web-->>API: AuthedApiError::<variant>
API-->>Up: Err(AuthedApiError)
else Success
Web-->>API: Response
API-->>Up: Ok(data)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
340-344
: Unauthenticated path should return a typed RecordingAction (not a String error).Returning Err("Please sign in...") bypasses the new typed flow and likely shows a generic error dialog instead of the sign‑in UI. Return RecordingAction::InvalidAuthentication for consistency.
- _ => { - // User is not signed in - return Err("Please sign in to use instant recording".to_string()); - } + _ => { + // User is not signed in + return Ok(RecordingAction::InvalidAuthentication); + }
🧹 Nitpick comments (10)
apps/desktop/src/utils/recording.ts (1)
42-47
: Normalize caught error to string for dialog.Pass a stringified error to avoid [object Object] in UI.
- .catch((err) => - dialog.message(err, { + .catch((err) => + dialog.message( + err instanceof Error ? err.message : String(err), + { title: "Error starting recording", kind: "error", - }), + }, + ), );apps/desktop/src-tauri/src/upload_legacy.rs (1)
205-221
: Inconsistent error typing breaks auth/upgrade signaling.This function now returns AuthedApiError, but create_or_get_video and presigned_s3_put still return String and convert typed errors from authed_api_request into Strings. This loses “InvalidAuthentication”/“UpgradeRequired” semantics.
Refactor both helpers to return Result<_, AuthedApiError> and propagate 401/upgrade as typed errors.
Minimal signatures:
-pub async fn create_or_get_video(...) -> Result<S3UploadMeta, String> { +pub async fn create_or_get_video(...) -> Result<S3UploadMeta, AuthedApiError> {-async fn presigned_s3_put(...) -> Result<String, String> { +async fn presigned_s3_put(...) -> Result<String, AuthedApiError> {And map:
- 401 → AuthedApiError::InvalidAuthentication
- upgrade_required → AuthedApiError::UpgradeRequired
- other → AuthedApiError::Other(String)
Also applies to: 213-214
apps/desktop/src-tauri/src/lib.rs (2)
1088-1121
: Flatten control flow for clarity (optional).Replace the inner async { ... }.await with directly awaiting create_or_get_video and matching its result.
- let s3_config = match async { - // build video_id... - create_or_get_video(...).await - }.await { + let s3_config = match { + // build video_id... + create_or_get_video(...).await + } { Ok(data) => data, Err(AuthedApiError::InvalidAuthentication) => return Ok(UploadResult::NotAuthenticated), Err(AuthedApiError::UpgradeRequired) => return Ok(UploadResult::UpgradeRequired), Err(err) => return Err(err.to_string()), };
1605-1617
: Consider preserving auth error semantics (optional).Converting AuthedApiError to String here loses “not authenticated” vs “upgrade required” distinctions. If UI should react, map them explicitly; otherwise this is fine.
apps/desktop/src-tauri/src/recording.rs (1)
316-328
: Handle all auth-related AuthedApiError variants.Only InvalidAuthentication and UpgradeRequired are mapped. If AuthedApiError gains other auth variants (e.g., token expired/not authenticated), they’ll fall through as generic errors and break the intended UX.
Please confirm all auth failures are mapped to RecordingAction in this match. If ManagerExt already normalizes all 401/403s to InvalidAuthentication, note that here or add additional match arms accordingly.
apps/desktop/src-tauri/src/api.rs (1)
32-39
: Map 401/403 to InvalidAuthentication for consistent typed errors across API calls.Right now, non‑2xx errors are returned as stringified messages. Promote auth status codes to AuthedApiError::InvalidAuthentication so callers can uniformly react (e.g., show sign‑in) without parsing strings.
Sample for upload_multipart_initiate (apply same pattern to other blocks):
+ use reqwest::StatusCode; ... if !resp.status().is_success() { - let status = resp.status(); - let error_body = resp - .text() - .await - .unwrap_or_else(|_| "<no response body>".to_string()); - return Err(format!("api/upload_multipart_initiate/{status}: {error_body}").into()); + let status = resp.status(); + let error_body = resp.text().await.unwrap_or_else(|_| "<no response body>".to_string()); + return Err(match status { + StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => AuthedApiError::InvalidAuthentication, + _ => format!("api/upload_multipart_initiate/{status}: {error_body}").into(), + }); }If ManagerExt::authed_api_request already returns Err on 401/403, note that and keep the change minimal or skip it here.
Also applies to: 74-81, 146-153, 201-208, 234-241
apps/desktop/src-tauri/src/upload.rs (4)
20-20
: Remove unused import.sentry::types::Auth is unused and may fail clippy.
-use sentry::types::Auth;
68-68
: Avoid println!; use tracing.There’s already an info! below; drop this to keep logging consistent.
- println!("Uploading video {video_id}...");
238-251
: Return detailed error (status/body) from create_or_get_video and keep upgrade mapping.Current return loses context with "Unknown error...". Read the body once, check for upgrade_required, else include status/body.
- if response.status() != StatusCode::OK { - #[derive(Deserialize, Clone, Debug)] - pub struct CreateErrorResponse { - error: String, - } - - if let Ok(error) = response.json::<CreateErrorResponse>().await { - if error.error == "upgrade_required" { - return Err(AuthedApiError::UpgradeRequired); - } - } - - return Err("Unknown error uploading video".into()); - } + if response.status() != StatusCode::OK { + #[derive(Deserialize, Clone, Debug)] + pub struct CreateErrorResponse { error: String } + + let status = response.status(); + let body = response.text().await.unwrap_or_else(|_| "<no response body>".to_string()); + if let Ok(err) = serde_json::from_str::<CreateErrorResponse>(&body) { + if err.error == "upgrade_required" { + return Err(AuthedApiError::UpgradeRequired); + } + } + return Err(format!("create_or_get_video/{status}: {body}").into()); + }
221-232
: URL‑encode query params (name/meta).Raw interpolation can break on spaces/&/unicode. Prefer building the URL with proper query encoding.
Would you like a small helper that builds s3_config_url using url::Url and .query_pairs_mut()?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/desktop/src-tauri/src/api.rs
(10 hunks)apps/desktop/src-tauri/src/auth.rs
(0 hunks)apps/desktop/src-tauri/src/deeplink_actions.rs
(1 hunks)apps/desktop/src-tauri/src/hotkeys.rs
(1 hunks)apps/desktop/src-tauri/src/lib.rs
(8 hunks)apps/desktop/src-tauri/src/recording.rs
(8 hunks)apps/desktop/src-tauri/src/upload.rs
(14 hunks)apps/desktop/src-tauri/src/upload_legacy.rs
(3 hunks)apps/desktop/src-tauri/src/web_api.rs
(4 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx
(2 hunks)apps/desktop/src/routes/in-progress-recording.tsx
(2 hunks)apps/desktop/src/routes/target-select-overlay.tsx
(3 hunks)apps/desktop/src/utils/recording.ts
(1 hunks)apps/desktop/src/utils/tauri.ts
(3 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src-tauri/src/auth.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/hotkeys.rs
apps/desktop/src-tauri/src/deeplink_actions.rs
apps/desktop/src/routes/in-progress-recording.tsx
apps/desktop/src/routes/(window-chrome)/(main).tsx
apps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/src/utils/recording.ts
apps/desktop/src-tauri/src/web_api.rs
apps/desktop/src-tauri/src/recording.rs
apps/desktop/src-tauri/src/upload_legacy.rs
apps/desktop/src-tauri/src/lib.rs
apps/desktop/src-tauri/src/api.rs
apps/desktop/src-tauri/src/upload.rs
apps/desktop/src/utils/tauri.ts
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs
: Format Rust code usingrustfmt
and ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/hotkeys.rs
apps/desktop/src-tauri/src/deeplink_actions.rs
apps/desktop/src-tauri/src/web_api.rs
apps/desktop/src-tauri/src/recording.rs
apps/desktop/src-tauri/src/upload_legacy.rs
apps/desktop/src-tauri/src/lib.rs
apps/desktop/src-tauri/src/api.rs
apps/desktop/src-tauri/src/upload.rs
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}
: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/routes/in-progress-recording.tsx
apps/desktop/src/routes/(window-chrome)/(main).tsx
apps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/src/utils/recording.ts
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}
: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format
.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
apps/desktop/src/routes/(window-chrome)/(main).tsx
apps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/src/utils/recording.ts
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}
: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx
).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
apps/desktop/src/routes/(window-chrome)/(main).tsx
apps/desktop/src/routes/target-select-overlay.tsx
apps/desktop/src/utils/recording.ts
apps/desktop/src/utils/tauri.ts
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file tauri.ts
Do not edit auto-generated files named
tauri.ts
.
Files:
apps/desktop/src/utils/tauri.ts
🧬 Code graph analysis (11)
apps/desktop/src-tauri/src/hotkeys.rs (1)
apps/desktop/src-tauri/src/recording.rs (1)
restart_recording
(683-700)
apps/desktop/src-tauri/src/deeplink_actions.rs (1)
apps/desktop/src-tauri/src/recording.rs (2)
start_recording
(259-639)inputs
(88-93)
apps/desktop/src/routes/in-progress-recording.tsx (2)
apps/desktop/src/utils/recording.ts (1)
handleRecordingResult
(6-48)apps/desktop/src/utils/tauri.ts (1)
commands
(7-278)
apps/desktop/src/routes/(window-chrome)/(main).tsx (2)
apps/desktop/src/utils/recording.ts (1)
handleRecordingResult
(6-48)apps/desktop/src/utils/tauri.ts (1)
commands
(7-278)
apps/desktop/src/routes/target-select-overlay.tsx (2)
apps/desktop/src/utils/recording.ts (1)
handleRecordingResult
(6-48)apps/desktop/src/utils/tauri.ts (1)
commands
(7-278)
apps/desktop/src/utils/recording.ts (2)
apps/desktop/src/utils/tauri.ts (2)
RecordingAction
(433-433)commands
(7-278)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery
(115-152)
apps/desktop/src-tauri/src/web_api.rs (2)
apps/desktop/src-tauri/src/lib.rs (1)
tauri_specta
(1906-2020)apps/desktop/src-tauri/src/auth.rs (1)
get
(46-52)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (2)
RecordingAction
(433-433)UploadMeta
(467-467)apps/desktop/src-tauri/src/upload.rs (1)
create_or_get_video
(206-263)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (4)
AuthStore
(346-346)Plan
(425-425)UploadResult
(471-471)UploadMeta
(467-467)
apps/desktop/src-tauri/src/upload.rs (1)
apps/desktop/src-tauri/src/upload_legacy.rs (1)
upload_image
(326-380)
apps/desktop/src/utils/tauri.ts (1)
apps/desktop/src-tauri/src/recording.rs (1)
inputs
(88-93)
🔇 Additional comments (10)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
241-248
: Good centralization of recording result handling.Looks correct. Ensure handleRecordingResult treats "Started" as success (no dialog). See utils/recording.ts note.
apps/desktop/src-tauri/src/upload_legacy.rs (1)
320-324
: Typed error conversion looks good.Using .into() to convert String → AuthedApiError aligns with the new signature.
apps/desktop/src/utils/tauri.ts (1)
14-16
: Bindings match backend RecordingAction changes.Looks correct. Since this file is generated, keep edits in Rust/types and re-export.
Also applies to: 26-28, 433-434
apps/desktop/src-tauri/src/lib.rs (1)
1165-1178
: Failure handling is consistent and user-facing.Persists failure, notifies, and returns a descriptive error string. LGTM.
apps/desktop/src/utils/recording.ts (1)
13-36
: dialog.message usage is correct The API v2.4.0 supports custom-buttons as an object and returns the clicked label; yourbuttons
object is valid and no array conversion is needed.Likely an incorrect or invalid review comment.
apps/desktop/src-tauri/src/recording.rs (3)
638-638
: Typed success is good.Returning Ok(RecordingAction::Started) aligns with the new frontend handler.
684-687
: Restart typing aligned.restart_recording now mirrors start_recording’s typed Result. Good.
900-901
: JoinHandle error mapping looks correct.Converts JoinError and AuthedApiError into strings for local logging/handling without altering upstream types.
apps/desktop/src-tauri/src/upload.rs (1)
158-176
: Confirm error conversion for ok_or on file_name.ok_or("Invalid file path")? relies on From<&str> for AuthedApiError. If not implemented, switch to String and Into.
If needed:
- let file_name = file_path + let file_name: String = file_path .file_name() .and_then(|name| name.to_str()) - .ok_or("Invalid file path")? - .to_string(); + .ok_or_else(|| "Invalid file path".to_string())? + .to_string();apps/desktop/src-tauri/src/hotkeys.rs (1)
149-151
: HotkeyAction::RestartRecording drops RecordingAction—surface auth/upgrade cases
The.map(|_| ())
swallows theRecordingAction
(e.g.InvalidAuthentication
/UpgradeRequired
) returned byrestart_recording
; emit or forward this result to the frontend so auth or upgrade prompts aren’t lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the code 👍
Blocked on #1092
TODO:
create_or_get_video
call instart_recording
to properly show auth UIapi.rs
and use the newAuthedApiError
typeCloses #1114
Summary by CodeRabbit
New Features
Refactor